-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handling federation _entities queries without creating @Query #2180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me. I just don‘t quite get, what the resolvers are needed for. I’m happy to hear that you want to add documentation, Javadoc and tests.
addQueries(schemaBuilder); | ||
GraphQLObjectType resolversType = Config.get().isFederationEnabled() | ||
? buildResolvers() | ||
: GraphQLObjectType.newObject().name("Resolver").build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I get this right, the resolversType
is only used in the if
block below, so we could move it there and simplify the trinary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to federation builder block
@@ -124,7 +125,7 @@ public static GraphQLSchema bootstrap(Schema schema) { | |||
} | |||
|
|||
public static GraphQLSchema bootstrap(Schema schema, boolean skipInjectionValidation) { | |||
if (schema != null && (schema.hasOperations())) { | |||
if (schema != null && (schema.hasOperations()) || Config.get().isFederationEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (schema != null && (schema.hasOperations()) || Config.get().isFederationEnabled()) { | |
if (schema != null && schema .hasOperations() || Config.get().isFederationEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong here (...). I fixed it
// hack! For schema build success if queries are empty. | ||
// It will be overrides in Federation transformation | ||
addSdlQueryToSchema(schemaBuilder, queryRootType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I‘m not sure, if I got this right: graphql-java federation transformation is going to overwrite the _service
query that we add here? It‘s just a dummy that is required to make the schema builder happy? Maybe it would make the code flow more understandable, if the method was named addDummyQuery
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you right, i rename this
I found this #1585, #521
If you request a field
value extended field. It turns out you need to make 2 requests, by id (default and may be query), and id and name (value requires field name). |
It would probably be nice to have such an option.
and allow pass queries like
in one method. or maybe it is wrong idea. |
Unfortunately, I couldn’t figure out how to make one general type. Too much magic Needs simple changes in quarkus (add resolvers operations here) |
Hi @jmartisk, @t1. What do you think about it? |
Hi, sorry a bit busy, but I'll try to review at the beginning of next week... |
This will require changes that are present in #2184 |
85f61e8
to
e21b5bf
Compare
@jmartisk Hi. I’ve probably already tired you, but please look at this PR too. For this PR, it also wouldn’t hurt to add quarkus to the |
Ok, would you also please create a PR to my Quarkus branch (https://github.com/jmartisk/quarkus/tree/smallrye-graphql-2.11) with the Quarkus-side change and preferably also a simple test that runs with Quarkus? |
Added changes and a couple of simple tests to your fork and create PR. After accepting the PR for quarkus, need restart the pipeline here, to be sure everything is ok. |
Ok, merged the Quarkus part, now re-running the CI here... Thanks! |
4c0c47d
to
8a9c4d9
Compare
@jmartisk It seems like I did everything, made all the necessary corrections. |
Thanks! |
See #2110